[IT-4485] Add endpoint to emit current balances as CSV #50
Conversation
ConsoleCatzirl
changed the title
brucehoff
left a comment
brucehoff
left a comment
There was a problem hiding this comment.
Very nice updates to documentation.
Please add unit tests for _balance_dates()
| | priority_codes | Comma-separated list of numeric codes | | ||
| | limit | Integer | | ||
|
|
||
| Boolean values: any value other than "no", "off", or "false" will be interpreted |
There was a problem hiding this comment.
The description below seems to indicate that the default for these params is true? Why not just make it so that any value other than a case-insensitive true is considered false?
There was a problem hiding this comment.
The goal is for them to enable optional behavior, so not providing a param would be considered disabled, and setting any value except for explicit False values would be considered enabled. Also, this way the empty string is True, so just having ?show_other_code is enough to enable the behavior, instead of needing ?show_other_code=true.
ConsoleCatzirl
force-pushed
the
balances
branch
2 times, most recently
from
f890b32 to
19933ea
Compare
ConsoleCatzirl
force-pushed
the
balances
branch
4 times, most recently
from
93d6e4e to
ac18b92
Compare
|
🎉 All dependencies have been resolved ! |
There was a problem hiding this comment.
Pull Request Overview
This PR adds a new /balances endpoint that outputs CSV data of GL account balances, designed for FloQast integration. The endpoint dynamically calculates the target period (previous month during the first week, or month-to-date otherwise) and uses the GL segment of the chart of accounts instead of the Program segment used by existing endpoints.
Key changes:
- Added
/balancesAPI endpoint with CSV output format - Refactored chart of accounts fetching to support arbitrary segments (Program vs GL)
- Implemented dynamic period calculation based on current date
- Added comprehensive test coverage for new functionality
Reviewed Changes
Copilot reviewed 9 out of 12 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_handler.py | Extensive test additions for new balances functionality, period calculations, and updated mocking |
| template.yaml | Added new API route, updated parameter names, and enhanced S3/KMS permissions |
| mip_api/util.py | Added period calculation logic and improved parameter handling functions |
| mip_api/upstream.py | Refactored to support arbitrary chart segments and added trial balance API calls |
| mip_api/s3.py | Extracted caching logic into reusable functions with improved error handling |
| mip_api/chart.py | Split chart retrieval into Program and GL variants, updated function signatures |
| mip_api/balances.py | New module for balance processing and CSV formatting |
| mip_api/init.py | Updated main handler to support new endpoint and refactored parameter handling |
| README.md | Updated documentation to reflect new endpoint and parameter changes |
Comments suppressed due to low confidence (1)
brucehoff
left a comment
brucehoff
left a comment
There was a problem hiding this comment.
Thank you for addressing my requests.
zaro0508
left a comment
zaro0508
left a comment
There was a problem hiding this comment.
This PR is a big change and difficult to review because the refactor (in PR #52) is included. Please help out reviewers by rebasing onto master and push up a new commit so that the files diffs only show the difference for this specific change.
Prepare for a new API endpoint that will require a different segment of the chart of accounts. The pre-existing endpoints used the Program segment of the chart of accounts, but the new endpoint uses the GL segment of the chart of accounts. Refactor to allow arbitrary segments of the chart of accounts.
ConsoleCatzirl
force-pushed
the
balances
branch
from
8c36847 to
804a4c6
Compare
Add a new API endpoint `/balances` to output a CSV of GL account balances appropriate for sending to FloQast. During the first week of the month, process the balance for the previous month, otherwise process the month-to-date balance.
ConsoleCatzirl
force-pushed
the
balances
branch
from
804a4c6 to
a974149
Compare
zaro0508
left a comment
zaro0508
left a comment
There was a problem hiding this comment.
LGTM, but why does the CI run the coverall check twice?
|
that's an excellent question, that's kinda weird behavior. the website also sporadically fails to load for me, so I think they're having some sort of extended partial outage. this is one more way that coveralls has been kinda unreliable, and tbh I'm starting to regret not choosing CodeCov for code coverage. |
4 participants
Add a new API endpoint
/balancesto output a CSV of GL accountbalances appropriate for sending to FloQast.
During the first week of the month, process the balance for the previous
month, otherwise process the month-to-date balance.
The pre-existing endpoints used the Program segment of the chart of
accounts, but the new endpoint uses the GL segment of the chart of
accounts. Refactor fetching the chart of accounts to support arbitrary
segments.
Depends on #52